Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1452] update traits_for_enum to accept _prefix and _suffix #1513

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tjbarker
Copy link

@tjbarker tjbarker commented Sep 20, 2021

in response to Issue.

goal is to allow _prefix and _suffix for enum trait definitions to stay in line with ruby implementations
also will not define the standard enum (without prefix/suffix) will only define the ones with prefix/suffix

@jasonkarns
Copy link

I came just to report this issue but it already exists, yay!

Glad to see work addressing it. Though as a user, I wouldn't be excited the current implementation requires duplicating the enum configuration in both the model and the factory. I would have expected that the factory "inherits" the enum configuration from the model itself. Otherwise, the prefix/suffix config could drift from what is defined in the model.

@tjbarker
Copy link
Author

Hey jasonkarns.

not wanting to replicate enum config in factories and models makes sense. This implementation was chosen because the flip side of explicitly setting these values would require hooking into active record which currently does not store or make available enough information to build this out.

my reasoning for thinking it wouldn’t be that bad to use this is:

  • the factory only needs to define the enum and prefix/suffix. If you are inheriting the enum list from the model then it will continue to inherit. Change in model enums are usually due to adding values which would automatically be added to the factory’s trait list, it’s not often that the enum attribute name or prefix/suffix change
  • this will only be needed for enums with a prefix/suffix. Most enums don’t have these so will continue to be automatically added as traits to factories

Copy link
Collaborator

@composerinteralia composerinteralia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work! A few thoughts, but I think we should be able to support this.

spec/acceptance/enum_traits_spec.rb Outdated Show resolved Hide resolved
@@ -230,8 +274,11 @@ def trait(name, &block)
# those traits. When this argument is not provided, factory_bot will
# attempt to get the values by calling the pluralized `attribute_name`
# class method.
def traits_for_enum(attribute_name, values = nil)
@definition.register_enum(Enum.new(attribute_name, values))
def traits_for_enum(attribute_name, values = nil, _prefix: false, _suffix: false, **values_as_hash)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supporting both hash and keyword arguments is making this a bit complicated. I don't think we need to match the Rails enum method exactly here, since the use cases are a bit different (for example, I think passing the values to traits_for_enum as keyword arguments would be fairly uncommon, and it's also not something we've tested).

I'd rather accept a single values argument (like a hash or array), and keep the prefix and suffix keyword arguments separate. That also allows us to get rid of the ugly underscores.

So that would look like:

    def traits_for_enum(attribute_name, values = nil, prefix: false, suffix: false)
      @definition.register_enum(Enum.new(attribute_name, values, prefix: prefix, suffix: suffix))
    end

All of our tests still pass with this change (assuming we update the _prefix and _suffix keyword arguments to remove the underscores).

Copy link
Author

@tjbarker tjbarker Jun 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue here is not necessarily that key names are needed, but more that a third argument is needed for setting options, but the traits_for_enum allows for values to be nil (in which case the second arg passed becomes options, not values).
if traits_for_enum is invoked with values being passed in, a third arg with options works fine, but if values is not passed in, the "options" now are the second arg, meaning values (was meant to default to nil) and options (was meant to be passed in) are both jeopardised.

i can think of 2 options here,
1: the code update I've made, which looks good from an api usage point of view, but means traits_for_enum becomes messy, as it has to try to determine on the fly what the intention of the second arg is.

2: if "options" are being used, values is a key name too (eg: traits_for_enum(:foo, values: [:a, :b], prefix: true, suffix: true), which means the consumer of the api has to change their code (only when setting options, which are a new concept so all current usage continues to work) but will need to migrate over when wanting to set prefix/suffix.

also, making the change you suggested above fails the test on line 60 (because of the confusion about what teh second arg is)

@@ -113,6 +113,211 @@ def each(&block)
expect(task.status).to eq(trait_name)
end
end

context 'when prefix is used' do
context 'when values are a hash' do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do prefix and suffixes interact differently with a hash vs. list, or is there something specific we're trying to cover by testing against both? If not, I think we could choose one or the other to test with, since we've already got coverage of the various enumerable types.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the current form of the code, no they don't behave differently, however with hash being passed in one of the tests (enum_for_trait_spec.rb:60) was failing as the keynames of the hash didn't match with the prefix, suffix options, so i chose to leave the 2 different tests in (for now at least) to ensure i pushed a solution that worked for all implementation types of the api

@@ -12,14 +23,16 @@ def build_traits(klass)
end
end

attr_reader :attribute_name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we using this anywhere?

Copy link
Author

@tjbarker tjbarker Oct 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was being used in lib/factory_bot/definition.rb to help with not redefining traits in the automatically_register_defined_enums method, it is now used in the Definition#register_enum method as the key in the registered_enum hash

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is now also being used in the registered_enums hash as the key to look up to ensure enums are not registered twice

lib/factory_bot/definition.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@aledustet aledustet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition! I think the change would be less "disruptive" if the new options become extra arguments there and we don't support the values as options signature but I'm excited to bring this in. Thank you!

#
# Equivalent to:
# factory :task do
# traits_for_enum :status, [:started, :finished], _suffix: 'status'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth pointing out we would need to update the docs here if we end up using the signature without the hash arguments.

lib/factory_bot/definition.rb Outdated Show resolved Hide resolved
…trait_for_enum to not take key names for options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants